Skip to content

Conversation

@syn-zhu
Copy link
Collaborator

@syn-zhu syn-zhu commented Aug 29, 2025

Description

https://jira.mongodb.org/browse/COMPASS-9499

Screen.Recording.2025-09-17.at.11.48.12.AM.mov

Implement saving and restoring of session tabs.

Filed https://jira.mongodb.org/browse/COMPASS-9913 to track a couple of potential followups that I'm omitting from this PR

TODO: add test cases:

  • there is no saved state.
  • one of the tabs we're trying to restore references a connection that no longer exists.
  • one of the tabs we're trying to restore no longer exists (e.g. collection / database was deleted);
  • user declines restore tabs

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@syn-zhu
Copy link
Collaborator Author

syn-zhu commented Sep 30, 2025

haven't fixed the logging comment yet but tried to address the other comments, @gribnoysup can you lmk if this looks better?

@syn-zhu syn-zhu requested a review from gribnoysup October 1, 2025 05:26
Comment on lines 231 to 235
loadSavedWorkspaces: async () => {
throwIfNotTestEnv();
return Promise.reject();
},
restoreSavedWorkspaces: () => throwIfNotTestEnv,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public interface of workspaces that you are adding this to, loading / restoring workspaces shouldn't be exposed through it: we shouldn't allow other parts of the app to trigger this process

@syn-zhu syn-zhu requested a review from gribnoysup October 4, 2025 11:57
@syn-zhu
Copy link
Collaborator Author

syn-zhu commented Oct 4, 2025

@gribnoysup Alright, I think it's pretty close now

@syn-zhu syn-zhu changed the title Smnzhu/save tab COMPASS-9499 Oct 5, 2025
@syn-zhu syn-zhu changed the title COMPASS-9499 feat(workspaces): Save and restore tabs COMPASS-9499 Oct 5, 2025
@github-actions github-actions bot added the feat label Oct 5, 2025
@syn-zhu syn-zhu added the no release notes Fix or feature not for release notes label Oct 5, 2025
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work well overall and aligned with all the architecture patterns nicely, good work! Still some code changes that I think we need to make. I mentioned this in one of the new review comments: this is a pretty big chunk of work and doing it in one go is just making it harder for yourself. We should add a feature flag and split the remaining work into smaller chunks

Also the e2e test failures are legit and need to be addressed, this feature now gets in the way of expected app behavior in some scenarios. Adding a FF will temporarily help to deal with that, but moving forward we should add some explicit e2e tests for a feature like that

* Schema for saving workspace tab state to user data
*/

// Define schema for collection subtab
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost everything here is exact copy of types defined in packages/compass-workspaces/src/types.ts, to avoid these diverging we should use one of those as source. As you'd need the schemas to use with UserData, we should probalby make schemas the source of types. This will require you to split the schemas here a bit to follow the type structure that we need in the app

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried to address this in 76ae816, lmk if that's better

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered in #7253 (comment)

Comment on lines 35 to 49
// Define schema for a workspace tab
export const WorkspaceTabSchema = z.object({
id: z.string(),
type: WorkspaceTabTypeSchema,
connectionId: z.string().optional(),
namespace: z.string().optional(),
initialQuery: z.record(z.any()).optional(),
initialAggregation: z.record(z.any()).optional(),
initialPipeline: z.array(z.record(z.any())).optional(),
initialPipelineText: z.string().optional(),
editViewName: z.string().optional(),
initialEvaluate: z.union([z.string(), z.array(z.string())]).optional(),
initialInput: z.string().optional(),
subTab: CollectionSubtabSchema.optional(),
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of relevant to my comment above: the schema here should be a tuple of values by workspace type, similar to how it's defined in the types file. That way zod will make sure that we will never end up with workspaces where properties are not matching the type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to address this and the comment above in 76ae816; Can you lmk if that's better?

@syn-zhu syn-zhu requested a review from gribnoysup October 11, 2025 18:29
Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question + one optional suggestion :shipit:

return;
}

void connections.connect(connectionInfo);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we deal with failures to connect?

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the feature is behind a feature flag, nothing immediately blocking this PR from my side (there are some legit failures in the CI, so please take a look). But a couple of things definitely need follow-ups. Can you please open tickets for those?

One thing that we discussed, but is still not addressed is the "invisible" tab titles while connections are loading:

Kapture.2025-10-22.at.10.27.21.mp4

As discussed before, this is due ConnectionInfoProvider rendering nothing while connection is in progress. Instead I think we should allow some sort of fallback rendering there and do this for titles inside the workspace to make it clear that we actually restored the tabs, they are just not ready.

The e2e tests are also missing and for a feature like that we definitely want some.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this file ever being used, why did you added it?


const CollectionSubtabSchema = z.enum(collectionSubtabValues);

export const WorkspaceTabSchema = z
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better, but only half way there IMO. As I said before, these types are now even more of a copy of what's defined in types.ts. Can you maybe walk me through your thought process here? Is there any reason you don't want to replace existing types in types.ts with types derived from the schemas? I don't see the reason not to do it, but maybe there's one I'm missing 🙂 Like imagine next time someone needs to add a new workspace, now they need to add exactly the same types twice in two different places, is there a good reason to do so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just don't understand what the suggestion is 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace existing types in types.ts with types derived from the schemas

How exactly do I actually do this? I think I'm getting a bit mixed up on the terminology here, can we maybe refer to specific file / class names?

Copy link
Collaborator Author

@syn-zhu syn-zhu Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to follow the suggestion but I don't actually understand what the suggestion is

It sounds like you're implying that there's some way to convert a typescript type (from types.ts) "directly" into a zod schema but I'm just not aware of how you would do that

@syn-zhu syn-zhu requested a review from gribnoysup October 24, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat no release notes Fix or feature not for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants